Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Major update of Telescope class #631

Merged
merged 53 commits into from
Nov 28, 2024
Merged

Major update of Telescope class #631

merged 53 commits into from
Nov 28, 2024

Conversation

anawas
Copy link
Collaborator

@anawas anawas commented Nov 7, 2024

Baseline functions of the Telescope class (e.g. max_baseline or get_stations_wgs84) now have the same output for OSKAR and RASCIL backends. Thus, the two code examples in issue #629 now work as expected. The main work was done in Telescope::constructor() function.

Other things that have been changed or adapted:

  • Renamed function get_baselines_wgs84 to get_stations_wgs84 because it always returned coordinates of stations, not baselines
  • Replaced numpy.loadtxt() with more robust class method __read_layout_txt() (in Telescope::_get_station_info)
  • Added some tests cases to test_telescope_baselines.py
  • Fixed some typos and added few lines of comments.

anawas added 29 commits October 29, 2024 09:41
@anawas
Warning properly formatted (no newline)
6b2d2be
@anawas
Testing baseline functionality for OSKAR telescope 📀
b66dde5
@anawas
Added comment about which value for Earth radius astropy uses 🪲
ae119f5
@anawas
Added test for coordinate_helper functions 💬
ce8f34e
@anawas
Set Earth radius to more precise value ⭐
e910983
@anawas
using build in conversions for rad <-> deg 🚊
aff02b9
@anawas
Add test for east_north function 💒
6e14340
@anawas
parametrized test ➗
8668164
@anawas
removed print statement 😬
694a8d5
@anawas
Added documentation 👭
15f2886
@anawas
Make clear that backend is OSKAR 🚘
f881dee
@anawas
Must be able to set name of RASCIL telescope 🍴
eb8cbda
@anawas
Add test to assert name can be set. 📠
ea96a8c
@anawas
Fixed typo 😐
21198dc
@anawas
Function to convert RASCIl to OSKAR 🎽
43fe31d
@anawas
Function creates KARABO telescope from RASCIL dataset 😐
5b4ad5a
@anawas
First test for new RASCIL functionality 🔺
5d267ec
@anawas
Refactored to own method 🐗
d595dd1
@anawas
Calculating combinations is easier with itertools ➖
df047fd
@anawas
Better wording (a baseline has a length) 📹
12ce08e
@anawas
Renaming baselines -> stations because the values are coordinates of …
fce940c 
…stations (antennas) 🕜
@anawas
Refactored baseline code 🇨🇳
bb29d21
@anawas
Edit comment ↔️
f2b7fef
@anawas
Must set delimiter or we get an error 🌻
4621fc5
@anawas
Must set delimiter or we cannot read in wit _get_station_infos() 😆
76984df
@anawas
Function __read_layout_txt is more robust than np.loadtxt 👪
0ff4ad2
@anawas
Using karabo functions to create tmp folder. This is consistent with …
a63d514 
…rest of code. 🎂
@anawas
Edit test for create_baseline_cut_telescope 🌱
6dbbf8d
@anawas
Narrowing of type now done properly 👶
b4c77f7
Lukas113
Lukas113 requested changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@Lukas113 Lukas113 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Some comments to address.

The failing CI-pipeline is not related to this PR, thus the test is ignored for this review.

karabo/simulation/sky_model.py Outdated Show resolved Hide resolved
karabo/simulation/coordinate_helper.py Show resolved Hide resolved
karabo/simulation/telescope.py Outdated Show resolved Hide resolved
karabo/simulation/telescope.py Show resolved Hide resolved
karabo/simulation/telescope.py Outdated Show resolved Hide resolved
karabo/test/test_rascil_telescope_setup.py Outdated Show resolved Hide resolved
karabo/test/test_telescope_baselines.py Show resolved Hide resolved
anawas added 11 commits November 19, 2024 17:56
@anawas
Copy link
Collaborator Author

anawas commented Nov 26, 2024 •
edited
Loading

I would like to close this pull request. I addressed most of the comments. But there is more work to do for the Telescope class (see issue #633). Tackling these issues here would clearly overload this PR. Thus I suggest to address the open comment regarding the telescope.add_antenna_to_station function there.

@Lukas113
Copy link
Collaborator

I would like to close this pull request. I addressed most of the comments. But there is more work to do for the Telescope class (see issue #633). Tackling these issues here would clearly overload this PR. Thus I suggest to address the open comment regarding the telescope.add_antenna_to_station function there.

What's the reason why you want this to be merged asap? Because of the release? I personally don't like to introduce potential misbehavior into main, when the issue should get addressed anyway. I am more in favor of leaving this PR open and you can focus on the other issue in a separate branch in case you want to merge it before the release.

@Lukas113 Lukas113 merged commit 420469a into main Nov 28, 2024
1 check passed
@Lukas113 Lukas113 deleted the aw_fix_telescope branch November 28, 2024 15:50
@anawas anawas mentioned this pull request Nov 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lukas113 Lukas113 Lukas113 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anawas @Lukas113